605192 - JM: make f.apply(x, obj) fast

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

We can apply the same speculation to js_fun_apply (bug 602129) as js_fun_call.  This is a prerequisite for bug 595884, which will speculatively avoid creating the args object.

Comment 1

[Luke Wagner [:luke]]

Attachment: fix JSStackFrame::initCallFrameX functions to match mjit

Comment 2

[Luke Wagner [:luke]]

Attachment: factor/optimize apply

This patch has preliminary changes to js_fun_apply that simplify the next patch.

Comment 3

[Luke Wagner [:luke]]

Attachment: WIP 1

This patch is lightly tested.

Preliminary results show a 3x (-m) speedup of
  function f(x) { return x+1 }
  f.apply(null, [3])
which puts us slightly ahead of jsc.  I think there is less relative speedup than bug 602129 due to the meatier nature of ic::SplatApplyArguments, which needs to be done in any case.

Comment 4

[Luke Wagner [:luke]]

Attachment: WIP 2

Passes trace/ref tests.  SS doesn't use much apply, but v8-raytrace (-m) does:

    raytrace:     1.199x as fast     294.5ms +/- 2.0%    245.7ms +/- 1.0%

(This is without jandem's optimization in bug 595884 to avoid creating args objects.)

Comment 5

[Luke Wagner [:luke]]

Attachment: swap a temp for saved reg

x64 was only providing a single SavedReg.  This patch makes ValueReg use r10 (a temp reg) which frees up r15.

Comment 6

[Luke Wagner [:luke]]

Attachment: WIP 3

Comment 7

[Luke Wagner [:luke]]

Attachment: factor/optimize apply 2

Comment 8

[Luke Wagner [:luke]]

Attachment: apply ic

Refactored and green on try.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 486279 [details] [diff] [review]
factor/optimize apply 2

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp

>+JSBool
>+js_GetArrayElements(JSContext *cx, JSObject *aobj, jsuint length, Value *vp)

This should use bool/true/false and be js:: rather than js_.  More importantly, GetArrayElements does not to me suggest it would work on anything but arrays.  GetElementsFromObject, perhaps, and add an interface-description comment by the declaration that makes explicit the requirement in the debug block at top of the method, maybe with a few words of rationalization?  (But you should add the comment regardless what name is chosen.)


>+{
>+#ifdef DEBUG
>+    jsuint maxlen;
>+    if (!js_GetLengthProperty(cx, aobj, &maxlen))
>+        return false;
>+    JS_ASSERT(length <= maxlen);
>+#endif

This introduces observably wrong behavior in debug builds for (untested) something like this:

  var called = false;
  var o = { get length() { if (!called) return called = true, 0; throw 42; } };
  Array.apply(undefined, o);

If you're going to do this, lookup the property or something and ware side effects getting the property might have.


>+    if (aobj->isDenseArray() && length <= aobj->getDenseArrayCapacity()) {
>+        Value *srcbeg = aobj->getDenseArrayElements();
>+        Value *srcend = srcbeg + length;
>+        for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src)
>+            *dst = src->isMagic(JS_ARRAY_HOLE) ? UndefinedValue() : *src;

Minor preference for src < srcend here, because in the case where somehow the fencepost is skipped, all values past it will also cause the loop to terminate.


>+        /*
>+         * Two cases, two loops: note how in the case of an active stack frame
>+         * backing aobj, even though we copy from fp->argv, we still must check
>+         * aobj->getArgsElement(i) for a hole, to handle a delete on the
>+         * corresponding arguments element. See args_delProperty.
>+         */
>+        JSStackFrame *fp = (JSStackFrame *) aobj->getPrivate();
>+        if (fp) {

No reason not to put the declaration in the if.


>+        } else {
>+            Value *srcbeg = aobj->getArgsElements();
>+            Value *srcend = srcbeg + length;
>+            for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src)
>+                *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src;

Same here.


>+        }
>+    } else {
>+        for (uintN i = 0; i < length; i++) {
>+            if (!aobj->getProperty(cx, INT_TO_JSID(jsint(i)), &vp[i]))
>+                return JS_FALSE;
>+        }

A kosher bounds-check, in my opinion.  :-)

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
Thanks, good catch on the debug-js_GetLengthProperty hazard.

> More importantly,
> GetArrayElements does not to me suggest it would work on anything but arrays. 

Chosen for parity with GetArrayElement.  Per real-life conversation, renaming both to GetElement/GetElements.

> >+            Value *srcbeg = aobj->getArgsElements();
> >+            Value *srcend = srcbeg + length;
> >+            for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src)
> >+                *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src;
> 
> Same here.

I generally prefer not to have overly-long for-initializers.  I think it makes this loop more readable, since the loop itself only talks about the loop variable and its bounds.

Comment 11

[Luke Wagner [:luke]]

Attachment: factor/optimize apply 3

Comment 12

[Luke Wagner [:luke]]

Waldo: ignore the unnamed namespace, I removed it.

Comment 13

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> > >+            Value *srcbeg = aobj->getArgsElements();
> > >+            Value *srcend = srcbeg + length;
> > >+            for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src)
> > >+                *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src;
> > 
> > Same here.
> 
> I generally prefer not to have overly-long for-initializers.  I think it makes
> this loop more readable, since the loop itself only talks about the loop
> variable and its bounds.

I saw after I made the comment that "Same here" bound to the wrong comment -- not to the != versus < comment I'd intended it to bind to, before adding an intervening comment.  So what I really meant (and hoped you'd intuit without having to burn a comment on it) was s/!=/</ and nothing more here.  Sorry for the confusion...

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 486802 [details] [diff] [review]
factor/optimize apply 3

Now GetElement/GetElements are inconsistent with SetOrDeleteArrayElement and SetArrayElement.  :-D  Please change them as well (no need for me to review those changes).

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp

>+        for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src)
>+            *dst = src->isMagic(JS_ARRAY_HOLE) ? UndefinedValue() : *src;

< rather than != still.


>+        } else {
>+            Value *srcbeg = aobj->getArgsElements();
>+            Value *srcend = srcbeg + length;
>+            for (Value *dst = vp, *src = srcbeg; src < srcend; ++dst, ++src)
>+                *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src;

...but you fixed this one, good.  :-)

r=me with the s/!=/</ and with the naming-consistency changes.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 486280 [details] [diff] [review]
apply ic

>+
>+    /*
>+     * Allocate nvals on the top of the stack, report error on failure.
>+     * N.B. the caller must ensure |from == firstUnused()|.

Is this now >= firstUnused() ?

>     DataLabelPtr assemble(void *ncode)
...
>+        } else {
>+            RegisterID newfp = tempRegs.takeAnyReg();
>+            masm.loadPtr(FrameAddress(offsetof(VMFrame, regs.sp)), newfp);

Comment here that SplatApplyArgs has been called, and regs.sp has been bumped to where the new JSStackFrame should lie.

>+    static bool isSaved(RegisterID reg) {
>+        uint32 mask = (1 << reg);

Instead, do Registers::maskReg(reg)

>+        RegisterID argcReg = (RegisterID)-1;  /* sssh gcc */

Prefer MaybeRegisterID for this.

>+        if (ic.frameSize.isStatic())
>+            masm.move(Imm32(ic.frameSize.staticArgc()), Registers::ArgReg1);
>+        else
>+            masm.move(argcReg, Registers::ArgReg1);

else if (argcReg != Registers::ArgReg1)

Nice patch! I like how it didn't require too many changes.

Comment 16

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/e77069ddab00
http://hg.mozilla.org/tracemonkey/rev/fd21a9d4344a
http://hg.mozilla.org/tracemonkey/rev/8715628a75f8
http://hg.mozilla.org/tracemonkey/rev/d9aceaabef28

Comment 17

[Jan de Mooij [:jandem]]

Landed a while ago:

http://hg.mozilla.org/mozilla-central/rev/e77069ddab00
http://hg.mozilla.org/mozilla-central/rev/fd21a9d4344a
http://hg.mozilla.org/mozilla-central/rev/8715628a75f8
http://hg.mozilla.org/mozilla-central/rev/d9aceaabef28

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e77069ddab00